Update feature added for headers command. #181
Conversation
| if lastCommitYear > info.EndYear || shouldUpdate { | ||
| // File was modified after copyright end year (or will be modified by us), update end year | ||
| // Use lastCommitYear if available, otherwise fall back to currentYear | ||
| targetEndYear := currentYear |
There was a problem hiding this comment.
End-year logic is not actually “git-aware”
So even if:
- a file was last changed in 2023, and
- there have been no commits touching that file since,
running the tool in 2026 will still change the header to end in 2026 ?
That means the header reflects when the tool was run, not when the file was last modified.
This contradicts:
- the README explanation, and
- the intent implied by function names like
GetFileLastCommitYear().
If the goal is “git-aware” and historically accurate headers, the end year should come from:
- the file’s last commit year (for source files), or
- the repo’s last commit year (for LICENSE),
and only fall back to the current year if git data isn’t available.
Right now, the behavior is effectively “always bump to now”, which can unintentionally rewrite large parts of a repo without any real code changes.
PLEASE VERIFY THIS!!!
There was a problem hiding this comment.
The logic is git aware currently.
There is no contradiction of what is mentioned in the readme vs what the code is actually doing.
The current behaviour for year-2 update is as follows:
- If copyright header's year-1 is updated due to change of
copyright_yearinfo in config file. - If the file's year-2 value is < last commit year of file.
In both cases, the year-2 value will be updated to current year as the last commit year of that file will be current year post header statement fix.
To answer your query,
So even if:
- a file was last changed in 2023, and
- there have been no commits touching that file since,
running the tool in 2026 will still change the header to end in 2026 ?
Ans -> No, the tool will not change the header to end in 2026. It will remain untouched.
Attached testing document which contains the test cases for the same.
Test_Copywrite_Repo.docx
There was a problem hiding this comment.
So, what I meant is that you need to clarify the ReamMe in a more clear way, here's my view:
The README says the end year is derived from git history (e.g: “End year: file’s last commit year” / “repo’s last commit year”). However, in the code (calculateYearUpdates), git history is only used to decide whether an update is needed, and when an update happens the end year is set to currentYear, not the git commit year.
It would be good to clarify this nuance explicitly in the README
There was a problem hiding this comment.
Sure ! Readme has been updated.
licensecheck/update.go
Outdated
| } | ||
|
|
||
| // GetFileLastCommitYear returns the year of the last commit that modified a file | ||
| func GetFileLastCommitYear(filePath string) (int, error) { |
There was a problem hiding this comment.
Potential issue with GetFileLastCommitYear()
The function runs git log from the file’s directory and passes only the filename. This is unreliable for nested paths, since Git resolves file paths relative to the repo root. In such cases, Git may return no commits or incorrect results, silently causing the logic to fall back to the current year and bump copyright years incorrectly.
There was a problem hiding this comment.
The current code walks through each file in each directory/subdirectory and covers all nested paths.
Code snippet here
However, for best practice, the fixed implementation is in place that runs from repo root with full relative paths.
|
Attached the testing document for the changes added : Test_Copywrite_Repo.docx |
CreatorHead
left a comment
CreatorHead
left a comment
There was a problem hiding this comment.
Could you please split this PR into smaller, focused ones and test each feature in isolation? It’ll make the review and validation much easier. Thanks!
3 participants
🛠️ Description
copywrite headersnow not only adds headers to files without headers but also updates year-1 and year-2 information in files with existing headers.copyright_holderinformation in the .copywrite.hcl file. copyright_holder value is 'IBM Corp.' by default.copywrite headerscommand if any copyright changes are detected in the current year from git commit history of the repo.copyright_yearvalue stored in the .copywrite.hcl file.copyright_holderandcopywrite licensecommand validation with respect to new changes made in year information.🔗 External Links
👍 Definition of Done
✅ New functionality works?
✅ Tests added?
🤔 Can be merged upon approval?
✅
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.